-
-
Notifications
You must be signed in to change notification settings - Fork 249
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support for SSE-based subscriptions for caliban-quick
#2141
Conversation
case ObjectValue((fieldName, StreamValue(stream)) :: Nil) => | ||
stream.either.map { | ||
case Right(r) => GraphQLResponse(ObjectValue(List(fieldName -> r)), resp.errors) | ||
case Left(err) => GraphQLResponse(ObjectValue(List(fieldName -> NullValue)), List(err)) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@paulpdaniels Is this error handling sound? Also, is there a better way to merge the error and success channels? I couldn't for the life of me figure it out 😕
val stream = (resp.data match { | ||
case ObjectValue((fieldName, StreamValue(stream)) :: Nil) => | ||
stream.either.map { | ||
case Right(r) => GraphQLResponse(ObjectValue(List(fieldName -> r)), resp.errors) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this saying: for all response bodies return the original errors? Would we want errors to be null here perhaps?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm I think you're right. It's probably better to send an initial one with all the errors (if they exist) and then the subsequent ones not containing errors
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made the the GraphQLResponse stream handling generic and placed it in HttpUtils in the core module so that both the caliban-quick and tapir adapters use the same one. Also the errors now are reported in the first event only
Followup of #2126.
@SvenW can you take a look and let me know if everything looks okay to you?